Organization dash cards tests migration (fixes : #2806) #2881
Conversation
…ashCards component and also merged css files into global css file
…ashCards component and also merged css files into global css file
WalkthroughThis pull request involves a comprehensive refactoring of CSS and component files within the Organization Dashboard components. The changes primarily focus on consolidating CSS styles from multiple module files into a single global Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/style/app.module.css (2)
804-827: Consider using relative units for typographyThe mobile styles look good, but consider using relative units (rem/em) consistently for better scalability and maintainability.
.cardBody .textWrapper .primaryText { - font-size: 1.5rem; + font-size: clamp(1.25rem, 4vw, 1.5rem); } .cardBody .textWrapper .secondaryText { - font-size: 1rem; + font-size: clamp(0.875rem, 3vw, 1rem); }
998-1012: Improve color management and layout flexibilityThe styles can be improved for better maintainability and flexibility:
- Replace hardcoded color with CSS variable
- Use more flexible width constraints
.cardItem .creator { font-size: 1rem; - color: rgb(33, 208, 21); + color: var(--bs-success, #198754); } .rightCard { display: flex; gap: 7px; - min-width: 170px; + min-width: min(170px, 30%); justify-content: center; flex-direction: column; margin-left: 10px; overflow-x: hidden; - width: 210px; + width: clamp(170px, 30%, 210px); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/OrganizationDashCards/CardItem.module.css(0 hunks)src/components/OrganizationDashCards/CardItem.spec.tsx(1 hunks)src/components/OrganizationDashCards/CardItem.tsx(1 hunks)src/components/OrganizationDashCards/CardItemLoading.spec.tsx(1 hunks)src/components/OrganizationDashCards/CardItemLoading.tsx(2 hunks)src/components/OrganizationDashCards/DashBoardCardLoading.spec.tsx(1 hunks)src/components/OrganizationDashCards/DashboardCard.spec.tsx(1 hunks)src/components/OrganizationDashCards/DashboardCard.tsx(1 hunks)src/components/OrganizationDashCards/DashboardCardLoading.tsx(2 hunks)src/components/OrganizationDashCards/Dashboardcard.module.css(0 hunks)src/style/app.module.css(2 hunks)
💤 Files with no reviewable changes (2)
- src/components/OrganizationDashCards/Dashboardcard.module.css
- src/components/OrganizationDashCards/CardItem.module.css
✅ Files skipped from review due to trivial changes (4)
- src/components/OrganizationDashCards/DashboardCard.spec.tsx
- src/components/OrganizationDashCards/CardItem.spec.tsx
- src/components/OrganizationDashCards/DashboardCard.tsx
- src/components/OrganizationDashCards/CardItem.tsx
🔇 Additional comments (14)
src/components/OrganizationDashCards/CardItemLoading.tsx (3)
2-2: Import path successfully updated to the global CSS.
This change aligns with the PR goal of consolidating styling into a single app.module.css file.
Line range hint 8-29: Component name refactored to PascalCase.
The use of PascalCase (CardItemLoading) is consistent with React best practices for component naming. Code looks concise and correct.
31-31: Export statement correctly reflects the updated component name.
This ensures consistency with the named function and prevents any potential import issues.
src/components/OrganizationDashCards/CardItemLoading.spec.tsx (3)
1-4: Imports are properly set up for new test file.
It references the updated CardItemLoading component and necessary testing libraries.
5-9: Test suite confirms component rendering.
The test checks for the cardItemLoading test ID, verifying that the component mounts successfully.
11-23: Thorough child elements test coverage.
This test verifies all key child elements (iconWrapper, title, etc.) are present, ensuring UI integrity.
src/components/OrganizationDashCards/DashboardCardLoading.tsx (4)
4-4: Global stylesheet import is consistent.
Switching to app.module.css follows the consolidation strategy described in the PR.
10-10: Component name update to PascalCase.
This naming convention aligns with standard React patterns and ensures clarity.
12-12: Added data-testid attribute for improved testability.
Facilitates targeted queries in the new test suite.
40-40: Default export matches the updated PascalCase name.
Maintaining a consistent name prevents confusion when importing.
src/components/OrganizationDashCards/DashBoardCardLoading.spec.tsx (4)
1-4: Imports set up correctly for new test suite.
All required dependencies, including local styles, are in place.
5-8: Pre-render in beforeEach ensures each test starts fresh.
This pattern keeps tests isolated and reliable.
9-11: Test verifies main component renders properly.
Asserting the presence of data-testid confirms the component’s visibility.
13-27: Comprehensive coverage of child elements.
Ensures each sub-element (e.g., cardBody, iconWrapper, themeOverlay) is rendered, confirming UI integrity.
|
Please resolve the conflicting file |
amaan-aly246
dismissed stale reviews from palisadoes and coderabbitai[bot]
via
cb432e7
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/style/app.module.css (1)
2016-2087: Use CSS variables for color consistencyThe creator text color is hardcoded to
rgb(33, 208, 21). For better maintainability and theme consistency, consider using a CSS variable.Apply this diff to use a CSS variable:
.cardItem .creator { font-size: 1rem; - color: rgb(33, 208, 21); + color: var(--bs-success, rgb(33, 208, 21)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(2 hunks)
🔇 Additional comments (2)
src/style/app.module.css (2)
1624-1647: LGTM! Well-implemented responsive design.
The media query for small screens effectively adjusts the card layout and typography for better mobile display. The absolute positioning of icons and text alignment changes provide an optimal viewing experience on smaller devices.
1649-1683: Ensure sufficient color contrast for accessibility
The secondary text color uses a CSS variable without a fallback value. Consider adding a fallback and ensuring the contrast ratio meets WCAG guidelines.
amaan-aly246
force-pushed
the
OrganizationDashCards_tests-migration
branch
from
cb432e7 to
fa7f183
Compare
amaan-aly246
force-pushed
the
OrganizationDashCards_tests-migration
branch
from
ee9cce1 to
e77e371
Compare
amaan-aly246
force-pushed
the
OrganizationDashCards_tests-migration
branch
from
aae108f to
daf829d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2881 +/- ##
=====================================================
+ Coverage 40.20% 88.39% +48.19%
=====================================================
Files 299 316 +17
Lines 7422 8273 +851
Branches 1624 1813 +189
=====================================================
+ Hits 2984 7313 +4329
+ Misses 4237 742 -3495
- Partials 201 218 +17 ☔ View full report in Codecov by Sentry. |
|
Please fix the conflicting file |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/style/app.module.css (1)
1624-1647: Consider improving mobile layout robustness.While the responsive design handles text sizing well, the absolute positioning of
.iconWrappercould cause layout issues.Apply this diff to make the layout more robust:
@media (max-width: 600px) { .cardBody { min-height: 120px; + position: relative; } .cardBody .iconWrapper { position: absolute; top: 1rem; left: 1rem; + z-index: 1; } .cardBody .textWrapper { - margin-top: calc(0.5rem + 36px); + margin-top: calc(0.5rem + 48px); text-align: right; + position: relative; + z-index: 0; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/OrganizationDashCards/CardItem.module.css(0 hunks)src/components/OrganizationDashCards/CardItem.spec.tsx(1 hunks)src/components/OrganizationDashCards/CardItem.tsx(4 hunks)src/components/OrganizationDashCards/CardItemLoading.spec.tsx(1 hunks)src/components/OrganizationDashCards/CardItemLoading.tsx(2 hunks)src/components/OrganizationDashCards/DashBoardCardLoading.spec.tsx(1 hunks)src/components/OrganizationDashCards/DashboardCard.spec.tsx(1 hunks)src/components/OrganizationDashCards/DashboardCard.tsx(1 hunks)src/components/OrganizationDashCards/DashboardCardLoading.tsx(2 hunks)src/components/OrganizationDashCards/Dashboardcard.module.css(0 hunks)src/style/app.module.css(2 hunks)
💤 Files with no reviewable changes (2)
- src/components/OrganizationDashCards/Dashboardcard.module.css
- src/components/OrganizationDashCards/CardItem.module.css
🚧 Files skipped from review as they are similar to previous changes (8)
- src/components/OrganizationDashCards/DashboardCard.spec.tsx
- src/components/OrganizationDashCards/DashboardCard.tsx
- src/components/OrganizationDashCards/CardItem.spec.tsx
- src/components/OrganizationDashCards/CardItemLoading.spec.tsx
- src/components/OrganizationDashCards/CardItemLoading.tsx
- src/components/OrganizationDashCards/CardItem.tsx
- src/components/OrganizationDashCards/DashboardCardLoading.tsx
- src/components/OrganizationDashCards/DashBoardCardLoading.spec.tsx
🔇 Additional comments (1)
src/style/app.module.css (1)
1649-1683: 🛠️ Refactor suggestion
Remove duplicate overlay styles and improve accessibility.
- The
.themeOverlayclass duplicates the overlay styles from.cardItem .iconWrapper .themeOverlay. - The secondary text color might not meet WCAG contrast requirements.
Apply these changes:
- Remove duplicate overlay styles:
- .cardBody .iconWrapper .themeOverlay {
- background: var(--bs-primary);
- position: absolute;
- top: 0;
- right: 0;
- bottom: 0;
- left: 0;
- opacity: 0.12;
- border-radius: 50%;
- }- Improve text contrast:
.cardBody .textWrapper .secondaryText {
font-size: 14px;
display: block;
- color: var(--bs-secondary);
+ color: var(--bs-secondary, #6c757d);
+ /* Ensure color meets WCAG AA contrast ratio of 4.5:1 */
}Likely invalid or redundant comment.
|
@palisadoes It's showing me that this branch is out-of-date with the |
|
Reopening. You don't have to do this. Just update your local branch with the latest upstream. |
I have requested your review on those unresolved conversations. |
|
I don't understand. You haven't responded to my queries about the deleted sections in the dialog boxes. |
|
@palisadoes I have replied to you in those dialog boxes. I mentioned there that CodeRabbit requested the deletion of those sections because they were duplicates, in order to resolve the conversation. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/style/app.module.css (2)
2390-2410: Remove duplicate text-overflow properties.The
.cardItem .titlehas duplicatetext-overflowandwhite-spaceproperties. Also, consider using the shared text truncation styles from the@supportsblock..cardItem .title { font-size: 1rem; flex: 1; overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; - text-overflow: ellipsis; - white-space: nowrap; /* Fallback for browsers that don't support line-clamp */ + text-overflow: ellipsis; + white-space: nowrap; margin-left: 3px; }
2411-2424: Remove duplicate line-clamp properties from location styles.The
.cardItem .locationduplicates line-clamp properties that are already defined in the@supportsblock above..cardItem .location { font-size: 0.9rem; color: var(--bs-primary); overflow: hidden; - display: -webkit-box; - -webkit-line-clamp: 1; - line-clamp: 1; - -webkit-box-orient: vertical; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(3 hunks)
🔇 Additional comments (5)
src/style/app.module.css (5)
1624-1647: LGTM! Responsive design implementation looks good.
The media query properly handles layout adjustments for smaller screens, with appropriate positioning and sizing of elements.
1649-1683: Remove duplicate overlay styles between .cardBody and .cardItem.
The .cardBody .iconWrapper .themeOverlay styles are duplicated with .cardItem .iconWrapper .themeOverlay. Consider extracting these into a shared class.
+.themeOverlay {
+ background: var(--bs-primary);
+ position: absolute;
+ top: 0;
+ right: 0;
+ bottom: 0;
+ left: 0;
+ opacity: 0.12;
+ border-radius: 50%;
+}
.cardBody .iconWrapper .themeOverlay {
- background: var(--bs-primary);
- position: absolute;
- top: 0;
- right: 0;
- bottom: 0;
- left: 0;
- opacity: 0.12;
- border-radius: 50%;
+ composes: themeOverlay;
}
.cardItem .iconWrapper .themeOverlay {
- background: var(--bs-primary);
- position: absolute;
- top: 0;
- right: 0;
- bottom: 0;
- left: 0;
- opacity: 0.12;
- border-radius: 50%;
+ composes: themeOverlay;
}2337-2344: LGTM! Card item layout implementation is solid.
The flexbox layout with proper spacing and border handling using CSS variables is well implemented.
2370-2388: LGTM! Icon wrapper implementation is clean.
The icon wrapper properly handles centering and dimensions while maintaining consistency with the design system.
2425-2454: Remove duplicate animation keyframes from .rightCard.
The zoomIn keyframes are duplicated within the .rightCard class. These animations already exist in the global scope.
.rightCard {
display: flex;
gap: 7px;
min-width: 170px;
justify-content: center;
flex-direction: column;
margin-left: 10px;
overflow-x: hidden;
width: 210px;
-
- @keyframes zoomIn {
- 0% {
- opacity: 0;
- -webkit-transform: scale(0.5);
- transform: scale(0.5);
- }
-
- 100% {
- opacity: 1;
- -webkit-transform: scale(1);
- transform: scale(1);
- }
- }
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/style/app.module.css (3)
1624-1647: Consider using a CSS variable for the breakpoint value.The media query implementation is good, but could be more maintainable by defining the breakpoint as a CSS variable.
+:root { + --breakpoint-sm: 600px; +} -@media (max-width: 600px) { +@media (max-width: var(--breakpoint-sm)) { .cardBody { min-height: 120px; } /* ... rest of the media query ... */ }
1679-1683: Add fallback color for better browser support.The secondary text color uses a CSS variable without a fallback value.
.cardBody .textWrapper .secondaryText { font-size: 14px; display: block; - color: var(--bs-secondary); + color: var(--bs-secondary, #6c757d); }
2391-2400: Remove duplicate text-overflow properties.The text truncation properties are duplicated.
.cardItem .title { font-size: 1rem; flex: 1; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; - text-overflow: ellipsis; - white-space: nowrap; /* Fallback for browsers that don't support line-clamp */ margin-left: 3px; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(3 hunks)
🔇 Additional comments (2)
src/style/app.module.css (2)
1653-1671: Remove duplicate overlay styles.
The .cardBody .iconWrapper overlay styles are duplicated with .cardItem .iconWrapper (lines 2371-2389).
2442-2454: 🛠️ Refactor suggestion
Remove duplicate animation keyframes.
The zoomIn keyframes are duplicated. They already exist in the global scope (lines 2344-2370).
.rightCard {
/* ... existing styles ... */
- @keyframes zoomIn {
- 0% {
- opacity: 0;
- -webkit-transform: scale(0.5);
- transform: scale(0.5);
- }
-
- 100% {
- opacity: 1;
- -webkit-transform: scale(1);
- transform: scale(1);
- }
- }
}Likely invalid or redundant comment.
amaan-aly246
force-pushed
the
OrganizationDashCards_tests-migration
branch
from
8ebcb99 to
c17efbf
Compare
amaan-aly246
force-pushed
the
OrganizationDashCards_tests-migration
branch
2 times, most recently
from
72dc159 to
2a46807
Compare
|
@palisadoes Don't know what's happening , I tried to resolved this issue using Issue: |
|
Suggest me what should I do now. |
|
Please update your local branch with the latest upstream. That should fix the issue. If not, please ask the #talawa-admin channel in slack |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/style/app.module.css (3)
2337-2360: Consider using CSS custom properties for breakpoints.The media query implementation is good, but could be more maintainable by defining breakpoints as CSS custom properties.
+:root { + --breakpoint-sm: 600px; + --breakpoint-md: 768px; + --breakpoint-lg: 1020px; +} -@media (max-width: 600px) { +@media (max-width: var(--breakpoint-sm)) { .cardBody { min-height: 120px; } /* ... rest of the media query ... */ }
2362-2384: Ensure sufficient contrast with overlay opacity.The overlay opacity of 0.12 might not provide sufficient contrast for accessibility. Consider adjusting the opacity or using a different approach for the overlay effect.
.cardBody .iconWrapper .themeOverlay { background: var(--bs-primary); position: absolute; top: 0; right: 0; bottom: 0; left: 0; - opacity: 0.12; + opacity: 0.15; /* Increased for better contrast */ border-radius: 50%; }
3140-3142: Improve color consistency and code organization.
- Use CSS variables consistently for colors instead of hard-coded values
- Consider grouping related component styles together
.cardItem .creator { font-size: 1rem; - color: rgb(33, 208, 21); + color: var(--bs-success); } .cardAddOnEntry { - border: 4px solid green; + border: 4px solid var(--bs-success); }Also applies to: 3182-3184
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(3 hunks)
🔇 Additional comments (1)
src/style/app.module.css (1)
3154-3166: 🛠️ Refactor suggestion
Remove duplicate animation keyframes.
The zoomIn keyframes are duplicated within the .rightCard class. These animations already exist in the global scope.
.rightCard {
/* ... other styles ... */
- @keyframes zoomIn {
- 0% {
- opacity: 0;
- -webkit-transform: scale(0.5);
- transform: scale(0.5);
- }
-
- 100% {
- opacity: 1;
- -webkit-transform: scale(1);
- transform: scale(1);
- }
- }
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/style/app.module.css (2)
3104-3131: Optimize text truncation implementationThe text truncation implementation can be improved for better maintainability and cross-browser compatibility:
- The
.cardItem .titlehas duplicate text-overflow properties- The
.cardItem .locationuses both modern and legacy line-clamp propertiesApply this diff to optimize the text truncation:
.cardItem .title { font-size: 1rem; flex: 1; overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; /* Fallback for browsers that don't support line-clamp */ margin-left: 3px; } .cardItem .location { font-size: 0.9rem; color: var(--bs-primary); overflow: hidden; - display: -webkit-box; - -webkit-line-clamp: 1; - line-clamp: 1; - -webkit-box-orient: vertical; }The
@supportsblock already handles the line clamping for modern browsers.
3138-3141: Use CSS variables consistently for colorsThe creator text color uses a hardcoded fallback color instead of using the CSS variable consistently.
Apply this diff to maintain consistency:
.cardItem .creator { font-size: 1rem; - color: var(--bs-success, #21d015); + color: var(--bs-success); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(3 hunks)
🔇 Additional comments (3)
src/style/app.module.css (3)
2337-2396: LGTM! Well-structured responsive styles
The media query and card body styles are well-organized with proper use of CSS variables and responsive design principles.
Line range hint 3050-3101: LGTM! Clean card item implementation
The card item and icon wrapper styles are well-implemented with proper use of flexbox, positioning, and theming through CSS variables.
3142-3185: 🛠️ Refactor suggestion
Fix structure issues in the right card styles
The .rightCard class has several issues:
- Contains duplicate
@keyframes zoomInanimation that already exists globally - Contains unrelated component styles (AddOnEntry.tsx)
- Missing closing brace
Apply this diff to fix the structure:
.rightCard {
display: flex;
gap: 7px;
min-width: 170px;
justify-content: center;
flex-direction: column;
margin-left: 10px;
overflow-x: hidden;
width: 210px;
}
- @keyframes zoomIn {
- 0% {
- opacity: 0;
- -webkit-transform: scale(0.5);
- transform: scale(0.5);
- }
- 100% {
- opacity: 1;
- -webkit-transform: scale(1);
- transform: scale(1);
- }
- }
/* Move these to a separate section for AddOnEntry component */
.entrytoggle {
margin: 24px 24px 0 auto;
width: fit-content;
}
.entryaction {
margin-left: auto;
display: flex !important;
align-items: center;
background-color: transparent;
color: #31bb6b;
}
.entryaction .spinner-grow {
height: 1rem;
width: 1rem;
margin-right: 8px;
}
- } /* Remove this extra closing brace */Likely invalid or redundant comment.
palisadoes
merged commit c364d0d
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
Migration of test cases written in Jest to Vitest and merged CSS files into
app.module.cssIssue Number:
Fixes #2806
Did you add tests for your changes?
Yes, additional test cases have been added for components that did not have test cases written in Jest.
Snapshots/Videos:
If relevant, did you update the documentation?
No
Does this PR introduce a breaking change?
No
Other information
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Tests
CardItemLoadingandDashBoardCardLoadingcomponents to verify rendering and child elements.Chores